Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added vlan_range support #396

Closed
wants to merge 32 commits into from
Closed

Added vlan_range support #396

wants to merge 32 commits into from

Conversation

Alopalao
Copy link

@Alopalao Alopalao commented Nov 6, 2023

Closes #18
Closes #309

Summary

This PR needs kytos PR
Added support for vlan_range epic. Currently only when both UNIs have the same list of tags

Local Tests

  • Created, updated and deleted circuit.
  • Restarted kytos to check consistency work. Partial updates for list of tags also tested. For example if both UNI have [[20, 30]] and they are being updated to [[25, 30]].
  • Added and updated tests.

End-To-End Tests

============================= test session starts ==============================
platform linux -- Python 3.9.2, pytest-7.2.0, pluggy-1.3.0
rootdir: /tests
plugins: timeout-2.1.0, rerunfailures-10.2, anyio-3.6.2
collected 244 items

tests/test_e2e_01_kytos_startup.py ..                                    [  0%]
tests/test_e2e_05_topology.py ..................                         [  8%]
tests/test_e2e_10_mef_eline.py ..........ss.....x.....x................  [ 24%]
tests/test_e2e_11_mef_eline.py ......                                    [ 27%]
tests/test_e2e_12_mef_eline.py .....Xx.                                  [ 30%]
tests/test_e2e_13_mef_eline.py ....Xs.s.....Xs.s.XXxX.xxxx..X........... [ 47%]
.                                                                        [ 47%]
tests/test_e2e_14_mef_eline.py x                                         [ 47%]
tests/test_e2e_15_mef_eline.py ....                                      [ 49%]
tests/test_e2e_20_flow_manager.py .....................                  [ 58%]
tests/test_e2e_21_flow_manager.py ...                                    [ 59%]
tests/test_e2e_22_flow_manager.py ...............                        [ 65%]
tests/test_e2e_23_flow_manager.py ..............                         [ 71%]
tests/test_e2e_30_of_lldp.py ....                                        [ 72%]
tests/test_e2e_31_of_lldp.py ...                                         [ 74%]
tests/test_e2e_32_of_lldp.py ...                                         [ 75%]
tests/test_e2e_40_sdntrace.py .............                              [ 80%]
tests/test_e2e_41_kytos_auth.py ........                                 [ 84%]
tests/test_e2e_42_sdntrace.py ..                                         [ 84%]
tests/test_e2e_50_maintenance.py ........................                [ 94%]
tests/test_e2e_60_of_multi_table.py .....                                [ 96%]
tests/test_e2e_70_kytos_stats.py ........                                [100%]

@Alopalao Alopalao requested a review from a team as a code owner November 6, 2023 23:11
models/evc.py Outdated
@@ -1480,6 +1623,7 @@ def handle_topology_update(self, switches: dict):
# All intra-switch EVCs do not have current_path.
# In case of inter-switch EVC and not current_path,
# link_down should take care of deactivation.
return
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return is here to avoid early activations so consistency checks can be tested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. you can add a TODO as well to remove it later.

Btw, this part is also being changed on #383

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alopalao as we spoke in the meeting, if you could git rebase with this branch fix/handle_switch_changes of #383, that'd be great. That way the linter error would also be gone. It was great how you managed to progress in the meantime.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased it and tested it, both are working well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Certain master originated commits here that haven't been merged into #383 is making the review slightly more difficult, if you could talk to @Ktmi to merge master into his branch, that'd be great.

Copy link
Member

@viniarck viniarck Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alopalao, I was checking David's PR commits history he's already got the correct commits there:

commit c9d91ed51799ce56e76e937d92bcb9cae6e1a9fb
Author: Vinicius Arcanjo 
Date:   Tue Oct 31 17:50:48 2023 -0300

    feat: add set_vlan only if in_vlan_a != in_vlan_z when pushing UNI flow
    related to issue 389


and on master same sha1:

commit c9d91ed51799ce56e76e937d92bcb9cae6e1a9fb
Author: Vinicius Arcanjo
Date:   Tue Oct 31 17:50:48 2023 -0300

    feat: add set_vlan only if in_vlan_a != in_vlan_z when pushing UNI flow
    related to issue 389

So you'll need to sort out why these commits got rewritten, if you need help with this let me know.

utils.py Show resolved Hide resolved
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alopalao, functionality-wise it's getting there. Excellent to this it working for the first time. I've hit an exception though, and also raised a few points.

Regarding the all 1's mask I've also opened an issue to look into it, and then we can make a final decision about whether or not to send with all 1s mask.

db/models.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
models/evc.py Show resolved Hide resolved
models/evc.py Outdated
@@ -1480,6 +1623,7 @@ def handle_topology_update(self, switches: dict):
# All intra-switch EVCs do not have current_path.
# In case of inter-switch EVC and not current_path,
# link_down should take care of deactivation.
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. you can add a TODO as well to remove it later.

Btw, this part is also being changed on #383

models/evc.py Show resolved Hide resolved
utils.py Show resolved Hide resolved
@viniarck viniarck requested review from italovalcy and a team November 7, 2023 15:21
@Alopalao
Copy link
Author

Alopalao commented Nov 9, 2023

Last commit is more stable. Tested with this script. This script works with the last update from kytos, topology and of_lldp PRs.

First change the vlan used by of_lldp, code, to None.
Runs script as python3 create_evcs.py 3. 3 is the number of circuits to be created. The script manages the number of ranges for tag_ranges.
Setting tag_range for "01:1" and "02:1" switches with the next values:

tag_ranges = []
for (i, j) in zip(range(1, 4096, 10), range(6, 4096, 10)):
    tag_ranges.append([i, j])

This tag_ranges goes from [[1, 6], [11, 16], [21, 26], ... to ..., [4061, 4066], [4071, 4076], [4081, 4086]]

The evcs created are from "01:1" and "02:1" interfaces. The VLANs values are calculated as follows:

for i in range(1, 6, 2):
    vlans = []
    for q in range(i, 4086, 10):
        vlans.append([q, q+1])
    t = create_evc(vlans_a=vlans, vlans_z=vlans)

The vlans for circuits are created as:

vlans_circuit_1 = [[1, 2], [11, 12], [21, 22], ..., [4061, 4062], [4071, 4072], [4081, 4082]]
vlans_circuit_2 = [[3, 4], [13, 14], [23, 24], ..., [4063, 4064], [4073, 4074], [4083, 4084]]
vlans_circuit_3 = [[5, 6], [15, 16], [25, 26], ..., [4065, 4066], [4075, 4076], [4085, 4086]]

It takes time to create all the circuit, but it is successful and with no warnings and errors. The final result should be an empty list for available_tags from "01:1" and "02:1" interfaces.

@@ -5,5 +5,5 @@
# pip-compile --output-file requirements/dev.txt requirements/dev.in
#
-e git+https://github.com/kytos-ng/python-openflow.git#egg=python-openflow
-e git+https://github.com/kytos-ng/kytos.git#egg=kytos[dev]
-e git+https://github.com/kytos-ng/kytos.git@epic/vlan_range#egg=kytos[dev]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverting this when PR lands

@viniarck viniarck self-requested a review November 14, 2023 17:31
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alopalao, great PR.

I've just done another pass closing the threads had I had opened. Fantastic to also see how you explored locally, and the new e2e that you're shipping. I'll do a final review once you also rebase with PR #383, let me know when you need a final review.

@viniarck
Copy link
Member

Changelog also hasn't been updated

@Alopalao Alopalao changed the base branch from master to fix/handle_switch_changes November 15, 2023 16:07
@Alopalao
Copy link
Author

Bypassing checking of tags for use_tags() and make_tags_available() since these tags are not managed by the user.
Commit a29489619e26a6ebea63ad69ce6c386992644dfc

@viniarck viniarck self-requested a review November 20, 2023 14:56
utils.py Outdated Show resolved Hide resolved
models/path.py Outdated Show resolved Hide resolved
models/evc.py Outdated Show resolved Hide resolved
@Alopalao Alopalao changed the base branch from fix/handle_switch_changes to master November 20, 2023 21:15
@Alopalao Alopalao changed the base branch from master to fix/handle_switch_changes November 20, 2023 21:16
@viniarck viniarck self-requested a review November 21, 2023 14:59
main.py Show resolved Hide resolved
@Alopalao
Copy link
Author

Alopalao commented Nov 21, 2023

Migrating to another branch epic/vlan_range_copy. This one have unnecessary commits. Changing it to draft so issues are not closed. New PR

@Alopalao Alopalao marked this pull request as draft November 21, 2023 18:10
Base automatically changed from fix/handle_switch_changes to master November 22, 2023 12:43
@viniarck
Copy link
Member

Closing this since Aldo's PR #407 has landed. Nicely done, Aldo.

@viniarck viniarck closed this Nov 22, 2023
@Alopalao Alopalao deleted the epic/vlan_range branch November 27, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update _prepare_uni_flows to deal with VLAN ranges Add support to VLAN range
2 participants